Simplify OrderbookSync constructor signature #177
Conversation
...instead of full instance of AuthenticatedClient
rmm5t
force-pushed
the
simplify-orderbook-sync-signature
branch
from
481193e
to
11cf861
Compare
| @@ -147,18 +141,17 @@ suite('OrderbookSync', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| test('emits an error event on error (with AuthenticatedClient)', done => { | |||
There was a problem hiding this comment.
Could one of these still use an authenticated client, to ensure we don't break backwards compatibility?
There was a problem hiding this comment.
Personally, I don't think we should have tests for that, because we're moving forward with the API. The benefit of previously changing the AuthenticatedClient properties and the backwards compatibility that comes with it is just a nice and elegant side effect . Backwards compatibility guarantees can be good sometimes, but they can also hold things back long-term.
Nonetheless, with that off my chest, I'm happy to add an extra test with AuthenticatedClient if would make you feel more comfortable. I'm just not sure this is the correct place to test that AuthenticatedClient quacks like an auth payload object of { key, secret, passphrase }.
There was a problem hiding this comment.
An extra test using an instance of AuthenticatedClient was just added via 09be1e8.
|
|
||
| this._queues = {}; // [] | ||
| this._sequences = {}; // -1 | ||
| this._public_clients = {}; | ||
| this.books = {}; | ||
|
|
||
| if (this.auth.secret) { | ||
| this._authenticatedClient = new AuthenticatedClient( |
There was a problem hiding this comment.
Can this default to auth if it already is an instance of AuthenticatedClient?
There was a problem hiding this comment.
I vote no...for the same reasons as above. We should not be making side-effect assumptions about the passed object, aside from the fact that it quacks like a { key, secret, passphrase } object. Otherwise, the project opens itself to a world of forwards compatibility issues and limitations down the road.
i.e. "Optimizations" like this only yield future maintenance problems. There's almost zero overhead to having an extra AuthenticatedClient instance.
e.g. If we did reuse a passed-in AuthenticationClient, the apiURI argument passed to the OrderbookSync constructor would then be ignored, and you'd end up with unintended consequences of talking to the wrong endpoint.
|
Awesome, thanks a lot! |
|
It appears to me that this commit goes in the opposite direction of dependency injection: https://en.wikipedia.org/wiki/Dependency_injection . Normally, one passes in all the objects that an object needs in its constructor and constructors don't construct other objects. It's ok to have to build an AuthenticatedClient and then pass it to a OrderbookSync even if the code is a little longer. This principle also makes it easier to pass in mock objects if one wanted to. Having a constructor construct objects itself removes this possibility. If you're concerned about the two URIs being out of sync, then just drop the URI argument. Unless there's something I'm missing, I suggest reverting this commit and only taking a PublicClient instance (which should have AuthenticatedClient as a sublcass, but that doesn't look like it was fixed until a week ago in 60f8014 ). So wait until gdax-tt upgrades to the latest gdax. |
This is a direct follow-up to #151 (comment)
checkAuth()utility function to ensure anauthobject containskey,secret, andpassphraseOrderbookSyncconstructor to take auth object instead of a fullAuthenticatedClientinstanceThis is still backwards compatible with the old constructor signature, because
AuthenticatedClienthas the samekey,secret, andpassphraseproperties./cc @fb55